Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move heap ownership info from chunk to pagemap #4371

Merged
merged 4 commits into from
Aug 8, 2023

Conversation

jemc
Copy link
Member

@jemc jemc commented Aug 1, 2023

This is a reopening of a PR from @dipinhora - PR #4368.

It was merged too soon (I didn't see the "do not merge" label).

dipinhora and others added 4 commits July 22, 2023 11:12
Prior to this commit, the chunk kept track of the heap ownership
info in the `chunk->actor` field. This worked but was not ideal
as noted in the `ponyint_heap_owner` function that this is a case
of false sharing where the chunk owner needs the chunk but
all other actors only need the chunk owner only. This led to lots
of pointer chasing as the pagemap was used to retrieve the chunk
pointerand then the chunk had to be loaded to retrieve the owning
actor pointer.

This commit removes the `chunk->actor` field further slimming down
both the `small_chunk` and `large_chunk` so both now fit within
32 bytes (on 64 bit architectures). The chunk ownership information
is now kept in the lowest level of the pagemap next to the chunk
information. This removes the previous pointer chasing because
the chunk owning actor pointer is retrieved at the same time as the
chunk pointer without needing to load the chunk itself. This is a
fairly standard tradeoff of memory for performance where we're now
storing more data in the pagemap to minimize pointer chasing. The
expectation is that this will have a net positive impact on
performance but no benchmarks have been run to validate this
assumption.
@SeanTAllen SeanTAllen added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Aug 1, 2023
@jemc jemc added do not merge This PR should not be merged at this time and removed changelog - changed Automatically add "Changed" CHANGELOG entry on merge labels Aug 1, 2023
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Aug 1, 2023
@SeanTAllen SeanTAllen added changelog - changed Automatically add "Changed" CHANGELOG entry on merge do not merge This PR should not be merged at this time discuss during sync Should be discussed during an upcoming sync and removed do not merge This PR should not be merged at this time discuss during sync Should be discussed during an upcoming sync labels Aug 1, 2023
@jemc
Copy link
Member Author

jemc commented Aug 1, 2023

From Sean:

We only want to merge this if #4369 doesn't have a straightforward fix we can implement quickly.

If that one does have a straightforward fix, we we want to fix that and release quickly. Otherwise, this PR can be merged and be subjected to a while of nightly stress tests before it gets released.

@jemc jemc merged commit 277329e into ponylang:main Aug 8, 2023
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Aug 8, 2023
@jemc jemc removed the do not merge This PR should not be merged at this time label Aug 8, 2023
github-actions bot pushed a commit that referenced this pull request Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants